[receiver/github] add workflow run event trace handling #37578
Conversation
github-actions
bot
requested review from
andrzej-stencel,
crobert-1 and
TylerHelmuth
| return | ||
| } | ||
| return | ||
| // TODO: Enable when handleWorkflowJob is implemented |
There was a problem hiding this comment.
Is there any issue we can link to here for tracking?
There was a problem hiding this comment.
The original GitHub receiver issue was auto closed due to staleness + having the GitHub scraping portion reach alpha stability since that was the original scope. Right now the only issue I have tracking this work is over in the SemConv CICD project. Would that work or would you rather I open a new issue in the Contrib space?
|
|
||
| traceID, err := newTraceID(e.GetWorkflowRun().GetID(), e.GetWorkflowRun().GetRunAttempt()) | ||
| if err != nil { | ||
| gtr.logger.Sugar().Errorf("Failed to generate trace ID", zap.String("error", fmt.Sprint(err))) |
There was a problem hiding this comment.
Should these error messages conform to standard error message requirements? (Referring here to the capitalization of "Failed")
| // newTraceID creates a deterministic Trace ID based on the provided | ||
| // inputs of runID and runAttempt. | ||
| func newTraceID(runID int64, runAttempt int) (pcommon.TraceID, error) { | ||
| // Original implementation appended `t` to TraceId's and `s` to |
There was a problem hiding this comment.
| // Original implementation appended `t` to TraceId's and `s` to | |
| // Original implementation appended `t` to TraceIds and `s` to |
| w.WriteHeader(http.StatusOK) | ||
| return | ||
| default: | ||
| gtr.logger.Sugar().Debugf("event type not supported", zap.String("event_type", eventType)) |
There was a problem hiding this comment.
Should the log level be a bit higher since this is an error, or is this an acceptable/expected problem that users usually don't care about?
There was a problem hiding this comment.
I'm a little torn on this one. Happy to get a second opinion. In the current state, this would trigger if the GitHub App (or repo webhook) was configured to send more than just workflow_{run, job} events. If it was emitted as an error, it could quickly clutter up the logs. The only thing to do would be to unsubscribe from those events.
On the http.error response, the only way someone is actually going to see that is If they opened up the GitHub app and looked at the last three days of event history. It's probably the right thing to do, but not that useful imo.
The only good alternative I can think of is setting:
- http.StatusAccepted (https://www.rfc-editor.org/rfc/rfc9110.html#section-15.3.3) 202 instead of error.
- and debug accepted but event not supported.
I do have plans to in the future had event transformation natively in there for other events, but that's not a soon problem.
Description
Adds functionality for creating a deterministic trace id and root span id coming from the workflow runs.
Link to tracking issue
Relates to #34944
Testing
Added tests and deployed locally.
Documentation
Updated the README to contain information about tracing.